Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

add dev clone to cli (#1421) #2229

Merged
merged 20 commits into from
Oct 14, 2023
Merged

add dev clone to cli (#1421) #2229

merged 20 commits into from
Oct 14, 2023

Conversation

keyallis
Copy link
Contributor

@keyallis keyallis commented Oct 7, 2023 •
edited
Loading

acorn-io/manager#1421

Here's a brief description of the flow so far:

  • On build we store the location (relative to the base of the git repo) and name of the Acornfile in the vcs information on the running app. We also store the build context.
  • If no vcs info is found on the running app then we don't support clone since we have no way of determining where to pull the source code from. We also don't support images that are missing the acornfile or buildContext fields.
  • We clone using the user's configured git cli. The name of the directory we create is based on the name of the running application.
  • We write the built Acornfile into the repository, and then warn the user if it differs from the one pulled down.

Checklist

  • The title of this PR would make a good line in Acorn's Release Note's Changelog
  • The title of this PR ends with a link to the main issue being address in parentheses, like: This is a title (#1216). Here's an example
  • All relevant issues are referenced in the PR description. NOTE: don't use GitHub keywords that auto-close issues
  • Commits follow contributing guidance
  • Automated tests added to cover the changes. If tests couldn't be added, an explanation is provided in the Verification and Testing section
  • Changes to user-facing functionality, API, CLI, and upgrade impacts are clearly called out in PR description
  • PR has at least two approvals before merging (or a reasonable exception, like it's just a docs change)

cjellick
cjellick reviewed Oct 7, 2023
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@keyallis keyallis requested a review from cjellick October 10, 2023 20:57
njhale
njhale reviewed Oct 10, 2023
View reviewed changes
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far!

pkg/apis/internal.acorn.io/v1/appimage.go Show resolved Hide resolved
pkg/cli/dev.go Outdated Show resolved Hide resolved
g-linville
g-linville reviewed Oct 11, 2023
View reviewed changes
pkg/apis/internal.acorn.io/v1/appimage.go Show resolved Hide resolved
pkg/cli/dev.go Outdated Show resolved Hide resolved
pkg/cli/dev.go Outdated Show resolved Hide resolved
pkg/cli/dev.go Outdated Show resolved Hide resolved
@keyallis keyallis requested review from g-linville and njhale October 11, 2023 16:56
tylerslaton
tylerslaton reviewed Oct 11, 2023
View reviewed changes
pkg/cli/dev.go Outdated Show resolved Hide resolved
Oscar Ward added 5 commits October 11, 2023 10:08
@keyallis keyallis requested a review from tylerslaton October 11, 2023 23:26
Oscar Ward added 6 commits October 12, 2023 08:50
thedadams
thedadams reviewed Oct 12, 2023
View reviewed changes
pkg/vcs/vcs.go Outdated Show resolved Hide resolved
pkg/vcs/vcs.go Outdated Show resolved Hide resolved
@keyallis keyallis requested a review from thedadams October 12, 2023 22:40
g-linville
g-linville approved these changes Oct 13, 2023
View reviewed changes
pkg/cli/dev.go Outdated Show resolved Hide resolved
pkg/vcs/vcs.go Outdated Show resolved Hide resolved
thedadams
thedadams reviewed Oct 13, 2023
View reviewed changes
pkg/vcs/vcs.go Outdated Show resolved Hide resolved
@keyallis keyallis requested a review from thedadams October 13, 2023 18:54
appease the linting gods
d18fb85 
Signed-off-by: Oscar Ward <[email protected]>
cjellick
cjellick reviewed Oct 13, 2023
View reviewed changes
pkg/vcs/vcs.go Outdated Show resolved Hide resolved
cjellick
cjellick reviewed Oct 13, 2023
View reviewed changes
pkg/vcs/vcs.go Outdated Show resolved Hide resolved
njhale
njhale reviewed Oct 13, 2023
View reviewed changes
pkg/vcs/vcs.go Show resolved Hide resolved
cjellick
cjellick reviewed Oct 13, 2023
View reviewed changes
pkg/vcs/vcs.go Outdated
// Directory is empty, clone the repo
err = gitClone(ctx, workdir, remote)
if err != nil {
fmt.Printf("%v\n", err)
Copy link
Member

@cjellick cjellick Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you customize these error messages a bit to make debugging easier? Like:
"Failed to clone remote %v. Skipping. Error: %v".

This applies to all the errors in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That customization is done in the function being called, let me know if you want more than what is there already.

cjellick
cjellick reviewed Oct 13, 2023
View reviewed changes
pkg/vcs/vcs.go Outdated Show resolved Hide resolved
thedadams
thedadams reviewed Oct 13, 2023
View reviewed changes
pkg/cli/dev.go Show resolved Hide resolved
pkg/vcs/vcs.go Outdated Show resolved Hide resolved
pr comments
253eb2d 
Signed-off-by: Oscar Ward <[email protected]>
njhale
njhale approved these changes Oct 14, 2023
View reviewed changes
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit, but otherwise lgtm

pkg/vcs/vcs.go Outdated Show resolved Hide resolved
@keyallis keyallis changed the title add dev clone to cli add dev clone to cli (#1421) Oct 14, 2023
add missing newline
c0e3ba9 
Signed-off-by: Oscar Ward <[email protected]>
@keyallis keyallis merged commit 28f2b00 into acorn-io:main Oct 14, 2023
2 of 3 checks passed
@keyallis keyallis deleted the dev-clone branch October 14, 2023 02:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@g-linville g-linville g-linville approved these changes

@njhale njhale njhale approved these changes

@ibuildthecloud ibuildthecloud Awaiting requested review from ibuildthecloud

@StrongMonkey StrongMonkey Awaiting requested review from StrongMonkey

@sosodev sosodev Awaiting requested review from sosodev

@iwilltry42 iwilltry42 Awaiting requested review from iwilltry42

@tylerslaton tylerslaton Awaiting requested review from tylerslaton

@cjellick cjellick Awaiting requested review from cjellick

@thedadams thedadams Awaiting requested review from thedadams

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@keyallis @cjellick @thedadams @njhale @g-linville @tylerslaton